Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix full/screen capture use last region #3364

Merged
merged 5 commits into from
Oct 10, 2023

Conversation

Kit-p
Copy link
Contributor

@Kit-p Kit-p commented Oct 5, 2023

Ignore "Use last region" option when running flameshot full or flameshot screen.

Closes #3086.

@mmahmoudian
Copy link
Member

Thanks for the PR. I believe instead of "exclusion" criteria we should only limit this to flameshot gui. This is because the flameshot screen should also ignore this as far as I can understand the use-case.

@Kit-p
Copy link
Contributor Author

Kit-p commented Oct 5, 2023

Thanks for the feedback. That makes a lot of sense.
In this case, it feels like we should also clarify it in the wordings.
Something like "Use last region for GUI capture"?

@Kit-p Kit-p changed the title Fix full capture use last region Fix full/screen capture use last region Oct 5, 2023
@mmahmoudian
Copy link
Member

In this case, it feels like we should also clarify it in the wordings.
Something like "Use last region for GUI capture"?

Yes, you right, the current wording is sub-optimal. Would you like to also include those changes in this PR (considering that it kinda fits the context)? What needs to be changed is the config window text and it's tooltip.

@Kit-p
Copy link
Contributor Author

Kit-p commented Oct 6, 2023

I am not sure how the translation system works, will changing the keys invalidate all current translations?

I can include it in this PR, but the translations should be a separate issue I suppose.

@Kit-p
Copy link
Contributor Author

Kit-p commented Oct 6, 2023

Okay, after some digging I believe here is what I need to do.
Please correct me if I misunderstand anything.

  1. Update the translation keys (English) in code.
  2. Replace the translation keys in all translation files so current translations are kept.
  3. Notify translators to help update the translations by setting type="unfinished". (?)

@mmahmoudian
Copy link
Member

As far as I understand, you don't need to care about translations. All you need to do is to change the code for English keys. Then weblate will pull changes from the master branch and automatically roll it as a string to be translated in all the supported languages.

This is also my understanding. I'm relatively new to handling translation conflicts (already a messy one has happened and I'm in the process of merging the translations manually between the master and the Weblate fork). For now, I have locked the Weblate from the admin dashboard, so it is safe change anything on our side.

@Kit-p
Copy link
Contributor Author

Kit-p commented Oct 8, 2023

Just tried changing English in code and compile with the GENERATE_TS option.
It set the current translation to have type="vanished" and added a new string to be translated.
I am just wondering if it would be better to keep the current ones, because the core meaning of the wordings hasn't deviated.

Btw, thanks a lot for the translation merging work, I definitely also noticed this messy situation.

@Kit-p
Copy link
Contributor Author

Kit-p commented Oct 9, 2023

Looks like this will be merged in the current state.
So attached is a patch for updating the translation files, feel free to ignore if it isn't helpful.
fix-flameshot-use-last-region-translation.patch

@veracioux
Copy link
Contributor

veracioux commented Oct 9, 2023

@Kit-p I've reviewed the patch file and it looks good. Although I can't apply it for some reason - it says error: translations/Internationalization_bg.ts: No such file or directory (repeated for each file in the patch).

I think you can commit the changes from the patch here, and we'll merge them all together. But let's wait for @mmahmoudian to confirm.

@mmahmoudian You mentioned a messy merge, are you talking about the Internationalization_ta.ts file between current weblate/master and master? If yes, it looks like that file effectively has no changes, but the whole file shows up as changed in the diff viewer. This is probably due to a change in windows/linux line endings. There are also some other conflicts, but not many and not especially tricky, unless I'm looking at the wrong thing.

@mmahmoudian
Copy link
Member

mmahmoudian commented Oct 9, 2023

@veracioux

You mentioned a messy merge, are you talking about the Internationalization_ta.ts file between current weblate/master and master

I think the file in question was *_fi.ts that was causing issues. This is what Weblate is reporting itself:

Rebasing (1/44)
Auto-merging data/translations/Internationalization_fi.ts
CONFLICT (content): Merge conflict in data/translations/Internationalization_fi.ts
error: could not apply 5aa5a5c... Translated using Weblate (Finnish)
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 5aa5a5c... Translated using Weblate (Finnish)

If you have the setup ready for merging please go ahead and merge. I haven't had time this week to work on it. If not, I will try to merge tomorrow.

@Kit-p
Copy link
Contributor Author

Kit-p commented Oct 9, 2023

@veracioux Ready for review, thanks!

@veracioux veracioux merged commit f5f2e53 into flameshot-org:master Oct 10, 2023
1 check passed
@veracioux
Copy link
Contributor

@Kit-p Merged. Thank you for your contribution!

@Kit-p Kit-p deleted the fix/full-ignore-use-last-region branch October 10, 2023 00:31
@mmahmoudian mmahmoudian added this to the v13 milestone Oct 10, 2023
panpuchkov pushed a commit to namecheap/flameshot that referenced this pull request Dec 22, 2023
* Fix full capture use last region

* Limit use last region to gui capture only

* Update config use last region wordings

* Preserve current translations for config use last region wordings

(cherry picked from commit f5f2e53)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flameshot full captures only part of the screen
3 participants